-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: add timeout in searching text in files #1233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9dec97e
to
e1d6814
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds timeout functionality to the findFiles
and findTextInFiles
tools to prevent them from hanging chat operations. The changes implement a 10-second timeout for both search operations and prompt rendering to improve user experience.
Key changes:
- Added timeout wrapper using
raceTimeout
andraceCancellation
utilities - Applied timeout to both file search operations and prompt rendering phases
- Added consistent error handling when timeouts occur
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
src/extension/tools/node/findTextInFilesTool.tsx | Added timeout logic to text search operations and prompt rendering with proper error handling |
src/extension/tools/node/findFilesTool.tsx | Added timeout logic to file search operations and prompt rendering with consistent error messaging |
const timeoutInMs = 10_000; | ||
const results = await raceTimeout( | ||
raceCancellation( | ||
Promise.resolve(this.searchService.findFiles(pattern, undefined, token)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Promise.resolve()
wrapper is unnecessary here. The findFiles
method likely already returns a Promise, so this adds unnecessary complexity.
Promise.resolve(this.searchService.findFiles(pattern, undefined, token)), | |
this.searchService.findFiles(pattern, undefined, token), |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the findFiles
returns a Thenable<>
which doesn't fit the required Promise<>
type, and fails compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make sense for the search service to return a Promise instead (this comes from our extension API but our searchService could do the Promise.resolve
. You can do that if it makes things cleaner, you don't have to
Please link the issue from the PR so we can keep track of what change goes with which issue. If you say |
Add timeout in findFiles and findTextInFiles tool so they won't hang the chat.
Fixes: microsoft/vscode#262743